[flang][OpenMP] Overhaul implementation of ATOMIC construct #137852
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The current implementation of the ATOMIC construct handles these clauses individually, and this change does not have an observable effect. At the same time these clauses are unique as per the OpenMP spec, and this patch reflects that in the OMP.td file.
The OpenMP implementation of the ATOMIC construct will change in the near future to accommodate OpenMP 6.0. This patch separates the shared implementations to avoid interfering with OpenACC.
The UPDATE clause can be specified on both ATOMIC and DEPOBJ directives. Currently, the ATOMIC directive has its own handling of it, and the definition of the UPDATE clause only supports its use in the DEPOBJ directive, where it takes a dependence-type as an argument. The UPDATE clause on the ATOMIC directive may not have any arguments. Since the implementation of the ATOMIC construct will be modified to use the standard handling of clauses, the definition of UPDATE should reflect that.
llvmbot
added
flang
|
@llvm/pr-subscribers-flang-parser @llvm/pr-subscribers-flang-openmp Author: Krzysztof Parzyszek (kparzysz) ChangesThe parser will accept a wide variety of illegal attempts at forming an ATOMIC construct, leaving it to the semantic analysis to diagnose any issues. This consolidates the analysis into one place and allows us to produce more informative diagnostics. The parser's outcome will be parser::OpenMPAtomicConstruct object holding the directive, parser::Body, and an optional end-directive. The prior variety of OmpAtomicXyz classes, as well as OmpAtomicClause have been removed. READ, WRITE, etc. are now proper clauses. The semantic analysis consistently operates on "evaluation" representations, mainly evaluate::Expr (as SomeExpr) and evaluate::Assignment. The results of the semantic analysis are stored in a mutable member of the OpenMPAtomicConstruct node. This follows a precedent of having Using a BLOCK construct containing multiple statements for an ATOMIC construct that requires multiple statements is now allowed. In fact, any nesting of such BLOCK constructs is allowed. This implementation will parse, and perform semantic checks for both conditional-update and conditional-update-capture, although no MLIR will be generated for those. Instead, a TODO error will be issues prior to lowering. The allowed forms of the ATOMIC construct were based on the OpenMP 6.0 spec. Patch is 243.18 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/137852.diff 34 Files Affected:
diff --git a/flang/include/flang/Parser/dump-parse-tree.h b/flang/include/flang/Parser/dump-parse-tree.h
index c0cf90c4696b6..a8c01ee2e7da3 100644
--- a/flang/include/flang/Parser/dump-parse-tree.h
+++ b/flang/include/flang/Parser/dump-parse-tree.h
@@ -526,15 +526,6 @@ class ParseTreeDumper {
NODE(parser, OmpAtClause)
NODE_ENUM(OmpAtClause, ActionTime)
NODE_ENUM(OmpSeverityClause, Severity)
- NODE(parser, OmpAtomic)
- NODE(parser, OmpAtomicCapture)
- NODE(OmpAtomicCapture, Stmt1)
- NODE(OmpAtomicCapture, Stmt2)
- NODE(parser, OmpAtomicCompare)
- NODE(parser, OmpAtomicCompareIfStmt)
- NODE(parser, OmpAtomicRead)
- NODE(parser, OmpAtomicUpdate)
- NODE(parser, OmpAtomicWrite)
NODE(parser, OmpBeginBlockDirective)
NODE(parser, OmpBeginLoopDirective)
NODE(parser, OmpBeginSectionsDirective)
@@ -581,7 +572,6 @@ class ParseTreeDumper {
NODE(parser, OmpDoacrossClause)
NODE(parser, OmpDestroyClause)
NODE(parser, OmpEndAllocators)
- NODE(parser, OmpEndAtomic)
NODE(parser, OmpEndBlockDirective)
NODE(parser, OmpEndCriticalDirective)
NODE(parser, OmpEndLoopDirective)
@@ -709,8 +699,6 @@ class ParseTreeDumper {
NODE(parser, OpenMPDeclareMapperConstruct)
NODE_ENUM(common, OmpMemoryOrderType)
NODE(parser, OmpMemoryOrderClause)
- NODE(parser, OmpAtomicClause)
- NODE(parser, OmpAtomicClauseList)
NODE(parser, OmpAtomicDefaultMemOrderClause)
NODE(parser, OpenMPDepobjConstruct)
NODE(parser, OpenMPUtilityConstruct)
diff --git a/flang/include/flang/Parser/parse-tree.h b/flang/include/flang/Parser/parse-tree.h
index e39ecc13f4eec..77f57b1cb85c7 100644
--- a/flang/include/flang/Parser/parse-tree.h
+++ b/flang/include/flang/Parser/parse-tree.h
@@ -4835,94 +4835,37 @@ struct OmpMemoryOrderClause {
CharBlock source;
};
-// 2.17.7 Atomic construct
-// atomic-clause -> memory-order-clause | HINT(hint-expression) |
-// FAIL(memory-order)
-struct OmpAtomicClause {
- UNION_CLASS_BOILERPLATE(OmpAtomicClause);
- CharBlock source;
- std::variant<OmpMemoryOrderClause, OmpFailClause, OmpHintClause> u;
-};
-
-// atomic-clause-list -> [atomic-clause, [atomic-clause], ...]
-struct OmpAtomicClauseList {
- WRAPPER_CLASS_BOILERPLATE(OmpAtomicClauseList, std::list<OmpAtomicClause>);
- CharBlock source;
-};
-
-// END ATOMIC
-EMPTY_CLASS(OmpEndAtomic);
-
-// ATOMIC READ
-struct OmpAtomicRead {
- TUPLE_CLASS_BOILERPLATE(OmpAtomicRead);
- CharBlock source;
- std::tuple<OmpAtomicClauseList, Verbatim, OmpAtomicClauseList,
- Statement<AssignmentStmt>, std::optional<OmpEndAtomic>>
- t;
-};
-
-// ATOMIC WRITE
-struct OmpAtomicWrite {
- TUPLE_CLASS_BOILERPLATE(OmpAtomicWrite);
- CharBlock source;
- std::tuple<OmpAtomicClauseList, Verbatim, OmpAtomicClauseList,
- Statement<AssignmentStmt>, std::optional<OmpEndAtomic>>
- t;
-};
-
-// ATOMIC UPDATE
-struct OmpAtomicUpdate {
- TUPLE_CLASS_BOILERPLATE(OmpAtomicUpdate);
- CharBlock source;
- std::tuple<OmpAtomicClauseList, Verbatim, OmpAtomicClauseList,
- Statement<AssignmentStmt>, std::optional<OmpEndAtomic>>
- t;
-};
-
-// ATOMIC CAPTURE
-struct OmpAtomicCapture {
- TUPLE_CLASS_BOILERPLATE(OmpAtomicCapture);
- CharBlock source;
- WRAPPER_CLASS(Stmt1, Statement<AssignmentStmt>);
- WRAPPER_CLASS(Stmt2, Statement<AssignmentStmt>);
- std::tuple<OmpAtomicClauseList, Verbatim, OmpAtomicClauseList, Stmt1, Stmt2,
- OmpEndAtomic>
- t;
-};
-
-struct OmpAtomicCompareIfStmt {
- UNION_CLASS_BOILERPLATE(OmpAtomicCompareIfStmt);
- std::variant<common::Indirection<IfStmt>, common::Indirection<IfConstruct>> u;
-};
-
-// ATOMIC COMPARE (OpenMP 5.1, OPenMP 5.2 spec: 15.8.4)
-struct OmpAtomicCompare {
- TUPLE_CLASS_BOILERPLATE(OmpAtomicCompare);
+struct OpenMPAtomicConstruct {
+ llvm::omp::Clause GetKind() const;
+ bool IsCapture() const;
+ bool IsCompare() const;
+ TUPLE_CLASS_BOILERPLATE(OpenMPAtomicConstruct);
CharBlock source;
- std::tuple<OmpAtomicClauseList, Verbatim, OmpAtomicClauseList,
- OmpAtomicCompareIfStmt, std::optional<OmpEndAtomic>>
+ std::tuple<OmpDirectiveSpecification, Block,
+ std::optional<OmpDirectiveSpecification>>
t;
-};
-// ATOMIC
-struct OmpAtomic {
- TUPLE_CLASS_BOILERPLATE(OmpAtomic);
- CharBlock source;
- std::tuple<Verbatim, OmpAtomicClauseList, Statement<AssignmentStmt>,
- std::optional<OmpEndAtomic>>
- t;
-};
+ // Information filled out during semantic checks to avoid duplication
+ // of analyses.
+ struct Analysis {
+ static constexpr int None = 0;
+ static constexpr int Read = 1;
+ static constexpr int Write = 2;
+ static constexpr int Update = Read | Write;
+ static constexpr int Action = 3; // Bitmask for None, Read, Write, Update
+ static constexpr int IfTrue = 4;
+ static constexpr int IfFalse = 8;
+ static constexpr int Condition = 12; // Bitmask for IfTrue, IfFalse
+
+ struct Op {
+ int what;
+ TypedExpr expr;
+ };
+ TypedExpr atom, cond;
+ Op op0, op1;
+ };
-// 2.17.7 atomic ->
-// ATOMIC [atomic-clause-list] atomic-construct [atomic-clause-list] |
-// ATOMIC [atomic-clause-list]
-// atomic-construct -> READ | WRITE | UPDATE | CAPTURE | COMPARE
-struct OpenMPAtomicConstruct {
- UNION_CLASS_BOILERPLATE(OpenMPAtomicConstruct);
- std::variant<OmpAtomicRead, OmpAtomicWrite, OmpAtomicCapture, OmpAtomicUpdate,
- OmpAtomicCompare, OmpAtomic>
- u;
+ mutable Analysis analysis;
};
// OpenMP directives that associate with loop(s)
diff --git a/flang/include/flang/Semantics/tools.h b/flang/include/flang/Semantics/tools.h
index f25babb3c1f6d..7f1ec59b087a2 100644
--- a/flang/include/flang/Semantics/tools.h
+++ b/flang/include/flang/Semantics/tools.h
@@ -782,5 +782,21 @@ inline bool checkForSymbolMatch(
}
return false;
}
+
+/// If the top-level operation (ignoring parentheses) is either an
+/// evaluate::FunctionRef, or a specialization of evaluate::Operation,
+/// then return the list of arguments (wrapped in SomeExpr). Otherwise,
+/// return the "expr" but with top-level parentheses stripped.
+std::vector<SomeExpr> GetOpenMPTopLevelArguments(const SomeExpr &expr);
+
+/// Both "expr" and "x" have the form of SomeType(SomeKind(...)[1]).
+/// Check if "expr" is
+/// SomeType(SomeKind(Type(
+/// Convert
+/// SomeKind(...)[2])))
+/// where SomeKind(...) [1] and [2] are equal, and the Convert preserves
+/// TypeCategory.
+bool IsSameOrResizeOf(const SomeExpr &expr, const SomeExpr &x);
+
} // namespace Fortran::semantics
#endif // FORTRAN_SEMANTICS_TOOLS_H_
diff --git a/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp b/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp
index b88454c45da85..0d941bf39afc3 100644
--- a/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp
+++ b/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp
@@ -332,26 +332,26 @@ getSource(const semantics::SemanticsContext &semaCtx,
const parser::CharBlock *source = nullptr;
auto ompConsVisit = [&](const parser::OpenMPConstruct &x) {
- std::visit(common::visitors{
- [&](const parser::OpenMPSectionsConstruct &x) {
- source = &std::get<0>(x.t).source;
- },
- [&](const parser::OpenMPLoopConstruct &x) {
- source = &std::get<0>(x.t).source;
- },
- [&](const parser::OpenMPBlockConstruct &x) {
- source = &std::get<0>(x.t).source;
- },
- [&](const parser::OpenMPCriticalConstruct &x) {
- source = &std::get<0>(x.t).source;
- },
- [&](const parser::OpenMPAtomicConstruct &x) {
- std::visit([&](const auto &x) { source = &x.source; },
- x.u);
- },
- [&](const auto &x) { source = &x.source; },
- },
- x.u);
+ std::visit(
+ common::visitors{
+ [&](const parser::OpenMPSectionsConstruct &x) {
+ source = &std::get<0>(x.t).source;
+ },
+ [&](const parser::OpenMPLoopConstruct &x) {
+ source = &std::get<0>(x.t).source;
+ },
+ [&](const parser::OpenMPBlockConstruct &x) {
+ source = &std::get<0>(x.t).source;
+ },
+ [&](const parser::OpenMPCriticalConstruct &x) {
+ source = &std::get<0>(x.t).source;
+ },
+ [&](const parser::OpenMPAtomicConstruct &x) {
+ source = &std::get<parser::OmpDirectiveSpecification>(x.t).source;
+ },
+ [&](const auto &x) { source = &x.source; },
+ },
+ x.u);
};
eval.visit(common::visitors{
diff --git a/flang/lib/Lower/OpenMP/OpenMP.cpp b/flang/lib/Lower/OpenMP/OpenMP.cpp
index fdd85e94829f3..ab868df76d298 100644
--- a/flang/lib/Lower/OpenMP/OpenMP.cpp
+++ b/flang/lib/Lower/OpenMP/OpenMP.cpp
@@ -2588,455 +2588,183 @@ genTeamsOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
//===----------------------------------------------------------------------===//
// Code generation for atomic operations
//===----------------------------------------------------------------------===//
+static fir::FirOpBuilder::InsertPoint
+getInsertionPointBefore(mlir::Operation *op) {
+ return fir::FirOpBuilder::InsertPoint(op->getBlock(),
+ mlir::Block::iterator(op));
+}
-/// Populates \p hint and \p memoryOrder with appropriate clause information
-/// if present on atomic construct.
-static void genOmpAtomicHintAndMemoryOrderClauses(
- lower::AbstractConverter &converter,
- const parser::OmpAtomicClauseList &clauseList, mlir::IntegerAttr &hint,
- mlir::omp::ClauseMemoryOrderKindAttr &memoryOrder) {
- fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder();
- for (const parser::OmpAtomicClause &clause : clauseList.v) {
- common::visit(
- common::visitors{
- [&](const parser::OmpMemoryOrderClause &s) {
- auto kind = common::visit(
- common::visitors{
- [&](const parser::OmpClause::AcqRel &) {
- return mlir::omp::ClauseMemoryOrderKind::Acq_rel;
- },
- [&](const parser::OmpClause::Acquire &) {
- return mlir::omp::ClauseMemoryOrderKind::Acquire;
- },
- [&](const parser::OmpClause::Relaxed &) {
- return mlir::omp::ClauseMemoryOrderKind::Relaxed;
- },
- [&](const parser::OmpClause::Release &) {
- return mlir::omp::ClauseMemoryOrderKind::Release;
- },
- [&](const parser::OmpClause::SeqCst &) {
- return mlir::omp::ClauseMemoryOrderKind::Seq_cst;
- },
- [&](auto &&) -> mlir::omp::ClauseMemoryOrderKind {
- llvm_unreachable("Unexpected clause");
- },
- },
- s.v.u);
- memoryOrder = mlir::omp::ClauseMemoryOrderKindAttr::get(
- firOpBuilder.getContext(), kind);
- },
- [&](const parser::OmpHintClause &s) {
- const auto *expr = semantics::GetExpr(s.v);
- uint64_t hintExprValue = *evaluate::ToInt64(*expr);
- hint = firOpBuilder.getI64IntegerAttr(hintExprValue);
- },
- [&](const parser::OmpFailClause &) {},
- },
- clause.u);
+static fir::FirOpBuilder::InsertPoint
+getInsertionPointAfter(mlir::Operation *op) {
+ return fir::FirOpBuilder::InsertPoint(op->getBlock(),
+ ++mlir::Block::iterator(op));
+}
+
+static mlir::IntegerAttr getAtomicHint(lower::AbstractConverter &converter,
+ const List<Clause> &clauses) {
+ fir::FirOpBuilder &builder = converter.getFirOpBuilder();
+ for (const Clause &clause : clauses) {
+ if (clause.id != llvm::omp::Clause::OMPC_hint)
+ continue;
+ auto &hint = std::get<clause::Hint>(clause.u);
+ auto maybeVal = evaluate::ToInt64(hint.v);
+ CHECK(maybeVal);
+ return builder.getI64IntegerAttr(*maybeVal);
}
+ return nullptr;
}
-static void processOmpAtomicTODO(mlir::Type elementType, mlir::Location loc) {
- if (!elementType)
- return;
- assert(fir::isa_trivial(fir::unwrapRefType(elementType)) &&
- "is supported type for omp atomic");
-}
-
-/// Used to generate atomic.read operation which is created in existing
-/// location set by builder.
-static void genAtomicCaptureStatement(
- lower::AbstractConverter &converter, mlir::Value fromAddress,
- mlir::Value toAddress,
- const parser::OmpAtomicClauseList *leftHandClauseList,
- const parser::OmpAtomicClauseList *rightHandClauseList,
- mlir::Type elementType, mlir::Location loc) {
- // Generate `atomic.read` operation for atomic assigment statements
- fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder();
+static mlir::omp::ClauseMemoryOrderKindAttr
+getAtomicMemoryOrder(lower::AbstractConverter &converter,
+ semantics::SemanticsContext &semaCtx,
+ const List<Clause> &clauses) {
+ std::optional<mlir::omp::ClauseMemoryOrderKind> kind;
+ unsigned version = semaCtx.langOptions().OpenMPVersion;
- processOmpAtomicTODO(elementType, loc);
-
- // If no hint clause is specified, the effect is as if
- // hint(omp_sync_hint_none) had been specified.
- mlir::IntegerAttr hint = nullptr;
-
- mlir::omp::ClauseMemoryOrderKindAttr memoryOrder = nullptr;
- if (leftHandClauseList)
- genOmpAtomicHintAndMemoryOrderClauses(converter, *leftHandClauseList, hint,
- memoryOrder);
- if (rightHandClauseList)
- genOmpAtomicHintAndMemoryOrderClauses(converter, *rightHandClauseList, hint,
- memoryOrder);
- firOpBuilder.create<mlir::omp::AtomicReadOp>(loc, fromAddress, toAddress,
- mlir::TypeAttr::get(elementType),
- hint, memoryOrder);
-}
-
-/// Used to generate atomic.write operation which is created in existing
-/// location set by builder.
-static void genAtomicWriteStatement(
- lower::AbstractConverter &converter, mlir::Value lhsAddr,
- mlir::Value rhsExpr, const parser::OmpAtomicClauseList *leftHandClauseList,
- const parser::OmpAtomicClauseList *rightHandClauseList, mlir::Location loc,
- mlir::Value *evaluatedExprValue = nullptr) {
- // Generate `atomic.write` operation for atomic assignment statements
- fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder();
+ for (const Clause &clause : clauses) {
+ switch (clause.id) {
+ case llvm::omp::Clause::OMPC_acq_rel:
+ kind = mlir::omp::ClauseMemoryOrderKind::Acq_rel;
+ break;
+ case llvm::omp::Clause::OMPC_acquire:
+ kind = mlir::omp::ClauseMemoryOrderKind::Acquire;
+ break;
+ case llvm::omp::Clause::OMPC_relaxed:
+ kind = mlir::omp::ClauseMemoryOrderKind::Relaxed;
+ break;
+ case llvm::omp::Clause::OMPC_release:
+ kind = mlir::omp::ClauseMemoryOrderKind::Release;
+ break;
+ case llvm::omp::Clause::OMPC_seq_cst:
+ kind = mlir::omp::ClauseMemoryOrderKind::Seq_cst;
+ break;
+ default:
+ break;
+ }
+ }
- mlir::Type varType = fir::unwrapRefType(lhsAddr.getType());
- // Create a conversion outside the capture block.
- auto insertionPoint = firOpBuilder.saveInsertionPoint();
- firOpBuilder.setInsertionPointAfter(rhsExpr.getDefiningOp());
- rhsExpr = firOpBuilder.createConvert(loc, varType, rhsExpr);
- firOpBuilder.restoreInsertionPoint(insertionPoint);
-
- processOmpAtomicTODO(varType, loc);
-
- // If no hint clause is specified, the effect is as if
- // hint(omp_sync_hint_none) had been specified.
- mlir::IntegerAttr hint = nullptr;
- mlir::omp::ClauseMemoryOrderKindAttr memoryOrder = nullptr;
- if (leftHandClauseList)
- genOmpAtomicHintAndMemoryOrderClauses(converter, *leftHandClauseList, hint,
- memoryOrder);
- if (rightHandClauseList)
- genOmpAtomicHintAndMemoryOrderClauses(converter, *rightHandClauseList, hint,
- memoryOrder);
- firOpBuilder.create<mlir::omp::AtomicWriteOp>(loc, lhsAddr, rhsExpr, hint,
- memoryOrder);
-}
-
-/// Used to generate atomic.update operation which is created in existing
-/// location set by builder.
-static void genAtomicUpdateStatement(
- lower::AbstractConverter &converter, mlir::Value lhsAddr,
- mlir::Type varType, const parser::Variable &assignmentStmtVariable,
- const parser::Expr &assignmentStmtExpr,
- const parser::OmpAtomicClauseList *leftHandClauseList,
- const parser::OmpAtomicClauseList *rightHandClauseList, mlir::Location loc,
- mlir::Operation *atomicCaptureOp = nullptr) {
- // Generate `atomic.update` operation for atomic assignment statements
- fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder();
- mlir::Location currentLocation = converter.getCurrentLocation();
+ // Starting with 5.1, if no memory-order clause is present, the effect
+ // is as if "relaxed" was present.
+ if (!kind) {
+ if (version <= 50)
+ return nullptr;
+ kind = mlir::omp::ClauseMemoryOrderKind::Relaxed;
+ }
+ fir::FirOpBuilder &builder = converter.getFirOpBuilder();
+ return mlir::omp::ClauseMemoryOrderKindAttr::get(builder.getContext(), *kind);
+}
+
+static mlir::Operation * //
+genAtomicRead(lower::AbstractConverter &converter, mlir::Location loc,
+ lower::StatementContext &stmtCtx, mlir::Value atomAddr,
+ const semantics::SomeExpr &atom, const semantics::SomeExpr &expr,
+ mlir::IntegerAttr hint,
+ mlir::omp::ClauseMemoryOrderKindAttr memOrder,
+ fir::FirOpBuilder::InsertPoint atomicAt,
+ fir::FirOpBuilder::InsertPoint prepareAt) {
+ fir::FirOpBuilder &builder = converter.getFirOpBuilder();
+ fir::FirOpBuilder::InsertPoint saved = builder.saveInsertionPoint();
+ builder.restoreInsertionPoint(prepareAt);
+
+ mlir::Value toAddr = fir::getBase(converter.genExprAddr(expr, stmtCtx, &loc));
+ mlir::Type atomType = fir::unwrapRefType(atomAddr.getType());
+
+ builder.restoreInsertionPoint(atomicAt);
+ mlir::Operation *op = builder.create<mlir::omp::AtomicReadOp>(
+ loc, atomAddr, toAddr, mlir::TypeAttr::get(atomType), hint, memOrder);
+ builder.restoreInsertionPoint(saved);
+ return op;
+}
- // Create the omp.atomic.update or acc.atomic.update operation
- //
- // func.func @_QPsb() {
- // %0 = fir.alloca i32 {bindc_name = "a", uniq_name = "_QFsbEa"}
- // %1 = fir.alloca i32 {bindc_name = "b", uniq_name = "_QFsbEb"}
- // %2 = fir.load %1 : !fir.ref<i32>
- // omp.atomic.update %0 : !fir.ref<i32> {
- // ^bb0(%arg0: i32):
- // %3 = arith.addi %arg0, %2 : i32
- // omp.yield(%3 : i32)
- // }
- // return
- // }
-
- auto getArgExpression =
- [](std::list<parser::ActualArgSpec>::const_iterator it) {
- const auto &arg{std::get<parser::ActualArg>((*it).t)};
- const auto *parserExpr{
- std::get_if<common::Indirection<parser::Expr>>(&arg.u)};
- return parserExpr;
- };
+static mlir::Operation * //
+genAtomicWrite(lower::AbstractConverter &converter, mlir::Location loc,
+ lower::StatementContext &stmtCtx, mlir::Value atomAddr,
+ const semantics::SomeExpr &atom, const semantics::SomeExpr &expr,
+ mlir::IntegerAttr hint,
+ mlir::omp::ClauseMemoryOrderKindAttr memOrder,
+ fir::FirOpBuilder::InsertPoint atomicAt,
+ fir::FirOpBuilder::InsertPoint prepareAt) {
+ fir::FirOpBuilder &builder = converter.getFirOpBuilder();
+ fir::FirOpBuilder::InsertPoint saved = builder.saveInsertionPoint();
+ builder.restoreInsertionPoint(prepareAt);
+
+ mlir::Value value = fir::getBase(converter.genExprValue(expr, stmtCtx, &loc));
+ mlir::Type atomType = fir::unwrapRefType(atomAddr.getType());
+ mlir::Value converted = builder.createConvert(loc, atomType, value);...
[truncated]
|
kparzysz
force-pushed
the
users/kparzysz/spr/a04-atomic-one
branch
from
fdc6376 to
bf39ff5
Compare
|
Previous PR: #137521 |
kparzysz
force-pushed
the
users/kparzysz/spr/a04-atomic-one
branch
from
bf39ff5 to
b345653
Compare
The parser will accept a wide variety of illegal attempts at forming an ATOMIC construct, leaving it to the semantic analysis to diagnose any issues. This consolidates the analysis into one place and allows us to produce more informative diagnostics. The parser's outcome will be parser::OpenMPAtomicConstruct object holding the directive, parser::Body, and an optional end-directive. The prior variety of OmpAtomicXyz classes, as well as OmpAtomicClause have been removed. READ, WRITE, etc. are now proper clauses. The semantic analysis consistently operates on "evaluation" represen- tations, mainly evaluate::Expr (as SomeExpr) and evaluate::Assignment. The results of the semantic analysis are stored in a mutable member of the OpenMPAtomicConstruct node. This follows a precedent of having `typedExpr` member in parser::Expr, for example. This allows the lowering code to avoid duplicated handling of AST nodes. Using a BLOCK construct containing multiple statements for an ATOMIC construct that requires multiple statements is now allowed. In fact, any nesting of such BLOCK constructs is allowed. This implementation will parse, and perform semantic checks for both conditional-update and conditional-update-capture, although no MLIR will be generated for those. Instead, a TODO error will be issues prior to lowering. The allowed forms of the ATOMIC construct were based on the OpenMP 6.0 spec.
kparzysz
force-pushed
the
users/kparzysz/spr/a04-atomic-one
branch
from
b345653 to
02b4080
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
This reverts commit 40510a3. Debug changes accidentally pushed upstream.
kparzysz
requested review from
mrkajetanp
and removed request for
Leporacanthicus
tblah
reviewed
May 28, 2025
There was a problem hiding this comment.
Thanks @kparzysz for the massive effort this patch must have taken to implement.
I have a few minor comments but I agree with your overall approach.
Right now I cannot build this with -Werror:
�[1m/home/$USER/llvm-project/flang/lib/Semantics/check-omp-structure.cpp:2820:56: �[0m�[0;1;31merror: �[0m�[1mmissing field 'iff' initializer [-Werror,-Wmissing-field-initializers]�[0m
2820 | GetActionStmt(std::get<parser::Block>(s.t))};�[0m
| �[0;1;32m ^
�[0m1 error generated.
ninja: build stopped: subcommand failed.
| // Generate `atomic.update` operation for atomic assignment statements | ||
| fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder(); | ||
| mlir::Location currentLocation = converter.getCurrentLocation(); | ||
| static mlir::IntegerAttr getAtomicHint(lower::AbstractConverter &converter, |
There was a problem hiding this comment.
It definately doesn't need to be done with this patch but I wonder if the clause processing can be moved to Clauses.cpp
Comment on lines
+50
to
+52
| ! An explicit conversion is accepted as an extension. | ||
| !$omp atomic update | ||
| x = int(x + y) |
There was a problem hiding this comment.
Please could you document this extension (maybe flang/docs/Extensions.md or at least explain in the commit message about these consequences of using evaluate::Expr).
flang/lib/Lower/OpenMP/OpenMP.cpp
Outdated
| //===----------------------------------------------------------------------===// | ||
|
|
||
| [[maybe_unused]] static void | ||
| dumpAnalysis(const parser::OpenMPAtomicConstruct::Analysis &analysis) { |
There was a problem hiding this comment.
Ultra-nit, but it isn't too far fetched that some other construct might one day need analysis following a similar pattern.
Suggested change
| dumpAnalysis(const parser::OpenMPAtomicConstruct::Analysis &analysis) { | |
| dumpAtomicAnalysis(const parser::OpenMPAtomicConstruct::Analysis &analysis) { |
|
|
||
| const SomeExpr &atom = *analysis.atom.get()->v; | ||
|
|
||
| llvm::errs() << "Analysis {\n"; |
There was a problem hiding this comment.
Please could you add a lit test for this
Comment on lines
780
to
796
|
|
||
| /// If the top-level operation (ignoring parentheses) is either an | ||
| /// evaluate::FunctionRef, or a specialization of evaluate::Operation, | ||
| /// then return the list of arguments (wrapped in SomeExpr). Otherwise, | ||
| /// return the "expr" but with top-level parentheses stripped. | ||
| std::vector<SomeExpr> GetOpenMPTopLevelArguments(const SomeExpr &expr); | ||
|
|
||
| /// Check if expr is same as x, or a sequence of Convert operations on x. | ||
| bool IsSameOrConvertOf(const SomeExpr &expr, const SomeExpr &x); | ||
|
|
||
| /// Strip away any top-level Convert operations (if any exist) and return | ||
| /// the input value. A ComplexConstructor(x, 0) is also considered as a | ||
| /// convert operation. | ||
| /// If the input is not Operation, Designator, FunctionRef or Constant, | ||
| /// is returns std::nullopt. | ||
| MaybeExpr GetConvertInput(const SomeExpr &x); | ||
|
|
There was a problem hiding this comment.
Should these be defined in tools.cpp not check-omp-structure.cpp?
There was a problem hiding this comment.
The implementations all use the "atomic" namespace in check-omp-structure.cpp, which I'd rather keep "private" to that file. If you think these declarations don't belong here, then maybe we could invent a new header omp-tools.h or something like that? Let me know what you think.
There was a problem hiding this comment.
I just thought it was confusing declaring them in a header with one name then defining them in a different cpp file. But you're right, it doesn't make a lot of sense putting them in check-omp-structure.h either.
I think these functions do not look very specific to atomic so it should be okay to extract them if you don't mind. Otherwise I am open to other ideas or if you feel strongly they can stay where they are. I don't think there's an actual style guide rule saying these can't be in a different header and cpp file.
There was a problem hiding this comment.
Done. I put the operation codes in an "operation" namespace in tools.h, and the implementations of the common functions in tools.cpp.
| Div, | ||
| Eq, | ||
| Eqv, | ||
| Ge, // extension |
There was a problem hiding this comment.
Do you think it is worth documenting these extensions somewhere?
|
Addressed a subset of comments, mostly in check-omp-structure.cpp. |
Don't emit diagnostics for those.
This allows emitting slightly better diagnostic messages.
| // The limit on the number of constructs is there to reduce the amount of | ||
| // unnecessary parsing when the end-directive is absent. It's higher than | ||
| // the maximum number of statements in any valid construct to accept cases | ||
| // when extra statements are present by mistake. |
There was a problem hiding this comment.
Is there a specific reason to allow a BodyLimit up to 5? Atomic structured blocks still have at most two statements (in case of capture construct). If we are speculating, wouldn't the end atomic construct (if it exists) occur at most 3 constructs later?
| // row or column, including the case where everything is zero. All these | ||
| // cases correspond to the determinant of the matrix being 0, which suggests | ||
| // that checking the det may be a convenient diagnostic check. There is only | ||
| // one additional case where the det is 0, which is when the matrx is all 1 |
There was a problem hiding this comment.
Nit: "matrx" -> "matrix"
| @@ -1,16 +0,0 @@ | |||
| ! RUN: not %flang_fc1 -fopenmp-version=51 -fopenmp %s 2>&1 | FileCheck %s | |||
There was a problem hiding this comment.
I think adding tests for unparsing would be good, since there are substantial changes to the parsing.
| !CHECK-NEXT: what: Read | ||
| !CHECK-NEXT: assign: v=x | ||
| !CHECK-NEXT: } | ||
| !CHECK-NEXT: op1 { |
There was a problem hiding this comment.
Nit: In cases where op1 is NULL, is it better to simply skip dumping it?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The parser will accept a wide variety of illegal attempts at forming an ATOMIC construct, leaving it to the semantic analysis to diagnose any issues. This consolidates the analysis into one place and allows us to produce more informative diagnostics.
The parser's outcome will be parser::OpenMPAtomicConstruct object holding the directive, parser::Body, and an optional end-directive. The prior variety of OmpAtomicXyz classes, as well as OmpAtomicClause have been removed. READ, WRITE, etc. are now proper clauses.
The semantic analysis consistently operates on "evaluation" representations, mainly evaluate::Expr (as SomeExpr) and evaluate::Assignment. The results of the semantic analysis are stored in a mutable member of the OpenMPAtomicConstruct node. This follows a precedent of having
typedExprmember in parser::Expr, for example. This allows the lowering code to avoid duplicated handling of AST nodes.Using a BLOCK construct containing multiple statements for an ATOMIC construct that requires multiple statements is now allowed. In fact, any nesting of such BLOCK constructs is allowed.
This implementation will parse, and perform semantic checks for both conditional-update and conditional-update-capture, although no MLIR will be generated for those. Instead, a TODO error will be issues prior to lowering.
The allowed forms of the ATOMIC construct were based on the OpenMP 6.0 spec.